-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(remix): Skip capturing ok
responses as errors.
#5405
Conversation
function captureRemixServerException(err: Error, name: string): void { | ||
// Skip capturing if the thrown error is a Response | ||
// https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders | ||
if (isResponse(err)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we still allow certain responses through? Like 500s? And then add the response data to the event also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. Maybe something like >=400
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds reasonable.
ok
responses as errors.
Fixes: #5425 Ref: #5429, #5405 As per review #5429 (comment), we need to define how and when we should capture a 4xx error. We will only capture thrown 5xx responses until then.
ref: #5362
Remix supports throwing responses from
loader
andaction
functions for its internalCatchBoundary
.They are catched on the caller level, but as we wrap the callees, they are registered as exceptions in the SDK. Being http responses, they end up like
{size: 0}
, which is not meaningful.This PR skips exception capturing for responses that are not
4xx
or5xx
.